Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow setting up an account with apppasword and folder via command-line arguments. For deployment. #5296

Merged
merged 4 commits into from
Dec 30, 2022
Merged

Allow setting up an account with apppasword and folder via command-line arguments. For deployment. #5296

merged 4 commits into from
Dec 30, 2022

Conversation

allexzander
Copy link
Contributor

@allexzander allexzander commented Dec 23, 2022 •
edited
Loading

Signed-off-by: alex-z blackslayer4@gmail.com

@allexzander allexzander changed the title Allow setting up an account with apppasword and folder via command-li… Allow setting up an account with apppasword and folder via command-line arguments. For deployment. Dec 23, 2022
@codecov
Copy link

codecov bot commented Dec 23, 2022 •
edited
Loading

Codecov Report

Merging #5296 (cecd24b) into master (399142f) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5296      +/-   ##
==========================================
+ Coverage   57.58%   57.69%   +0.11%     
==========================================
  Files         139      139              
  Lines       17595    17614      +19     
==========================================
+ Hits        10132    10163      +31     
+ Misses       7463     7451      -12
Impacted Files Coverage Δ
src/libsync/theme.cpp 17.32% <100.00%> (+3.21%) ⬆️
src/libsync/discovery.cpp 86.22% <0.00%> (+0.28%) ⬆️
src/libsync/propagateremotemove.cpp 82.18% <0.00%> (+0.57%) ⬆️
src/libsync/propagatedownload.cpp 64.90% <0.00%> (+1.17%) ⬆️

claucambra
claucambra requested changes Dec 27, 2022
View reviewed changes
Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It all looks good from at the code level, I think it just needs the restructuring we discussed

@allexzander allexzander force-pushed the feature/authentication-via-apppassword-argument branch 2 times, most recently from 7b10acb to 46ce7a1 Compare December 29, 2022 15:09
@allexzander allexzander marked this pull request as ready for review December 29, 2022 15:11
@allexzander allexzander force-pushed the feature/authentication-via-apppassword-argument branch from 7fe19fb to 991b333 Compare December 29, 2022 15:43
claucambra
claucambra reviewed Dec 29, 2022
View reviewed changes
Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some structural comments and a nitpick

src/gui/accountsetupcommandlinemanager.h Outdated Show resolved Hide resolved
src/gui/accountsetupcommandlinemanager.cpp Outdated Show resolved Hide resolved
src/gui/accountsetupcommandlinemanager.cpp Outdated Show resolved Hide resolved
src/gui/accountsetupcommandlinemanager.cpp Outdated Show resolved Hide resolved
@allexzander allexzander force-pushed the feature/authentication-via-apppassword-argument branch from 991b333 to 8f8dc27 Compare December 29, 2022 17:02
@allexzander allexzander force-pushed the feature/authentication-via-apppassword-argument branch 2 times, most recently from 551021a to 57fa421 Compare December 29, 2022 18:04
claucambra
claucambra reviewed Dec 30, 2022
View reviewed changes
Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-level wise looks good, just what we talked about regarding the static-ness of the manager and its members

@allexzander
Allow setting up an account with apppasword and folder via command-li…
94df676 
…ne arguments. For deployment.

Signed-off-by: alex-z <blackslayer4@gmail.com>
@allexzander
Fix review comments from Claudio. Refactoring.
e53d08f 
Signed-off-by: alex-z <blackslayer4@gmail.com>
@allexzander
Fix review comments from Claudio II.
07be556 
Signed-off-by: alex-z <blackslayer4@gmail.com>
@allexzander
Fix review comments from Claudio III.
cecd24b 
Signed-off-by: alex-z <blackslayer4@gmail.com>
@allexzander allexzander force-pushed the feature/authentication-via-apppassword-argument branch from a43aa54 to cecd24b Compare December 30, 2022 13:46
@allexzander
Copy link
Contributor Author

Code-level wise looks good, just what we talked about regarding the static-ness of the manager and its members

ok, changed it to singleton, take a look

@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5296-cecd24b6c526e087c7cfd2382fea4dc9eb7cba35-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

claucambra
claucambra approved these changes Dec 30, 2022
View reviewed changes
Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but please see my comment

src/gui/application.cpp Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Dec 30, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 36 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@claucambra
Copy link
Collaborator

Please clean up history before merging :)

@allexzander allexzander merged commit d2d9de7 into master Dec 30, 2022
@allexzander allexzander deleted the feature/authentication-via-apppassword-argument branch December 30, 2022 15:55
@allexzander
Copy link
Contributor Author

Please clean up history before merging :)

damn, forgot, well, ok what's done is done

@mgallien mgallien added this to the 3.7.0 milestone Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claucambra claucambra claucambra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@allexzander @nextcloud-desktop-bot @claucambra @mgallien